feat: Error Details Improvements - GRPC #1634

blakeli0 · 2022-03-15T22:02:45Z

This PR is for issue #1635
Key design notes:

All ApiException created for GRPC clients will go through GrpcApiExceptionFactory and the cause of ApiException already has all the info we need, hence the majority of the logic is in GrpcApiExceptionFactory.
Promoted all the fields of ErrorInfo to top level of ApiException, accessible through getters directly.
Encapsulated all the raw error messages from server to ErrorDetails. The unpacked error messages are accessible through getters.
If error messages from server are corrupted and unable to unpack, a ProtocolBufferParsingException will be thrown.

gax/src/main/java/com/google/api/gax/rpc/ApiException.java

gax/src/main/java/com/google/api/gax/rpc/ApiExceptionFactory.java

vam-google · 2022-03-16T18:34:54Z

gax/src/main/java/com/google/api/gax/rpc/CancelledException.java

+
+  public CancelledException(
+      Throwable cause, StatusCode statusCode, boolean retryable, ErrorDetails errorDetails) {
+    super(cause, statusCode, retryable, errorDetails);


Subjective: For this an all similar changes, can you make them such that each of the overloaded constructors call this() (except the last one, which does the job). This is an idiomatic way of doing it and is often easier to maintain

Do you mean making the first constructor call this(cause, statusCode, retryable, null)?

yep. It is a typical approach: to have some sort of a "master" consturctor and make other constructors simply call the big one. The master constructor can even be private, if we don't want to expose it.

I agree that this is ideal if possible, but I think it depends on whether super(field1, field2) has the same behavior as this(field1, field2, null), sometimes they don't. For example, super(cause) and this(null, cause) in RuntimeException have different behaviors, one will populate message from cause, the other will set it to null. That being said, since this is a side refactoring, maybe we can merge the main logic of the code first and come back to see if we can improve it.

gax/src/main/java/com/google/api/gax/rpc/ErrorDetails.java

gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcApiExceptionFactory.java

…ctory

eaball35 · 2022-03-21T19:15:17Z

gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcApiExceptionFactory.java

+    } catch (InvalidProtocolBufferException e) {
+      // If unpacking one of the error detail fails, it should not block unpacking other error
+      // details so that end users can still get some info back. Hence, we don't need to do
+      // anything.


If we just swallow this, if details are all corrupt we keep continuing and then create an empty error details and have no insight into what's going on?

Yes, in this case, an empty ErrorDetails will be created. I think an empty ErrorDetails is a legitimate case, it could happen either when server doesn't send anything or we are unable to unpack the message server sends.

Can we in his case just store the raw Any (not unpacked) detail object in ErrorDetails? Doing empyt error details will just loose information for users and make the error even more confusing to them.

Storing raw Any would not be useful to users in my opinion, they don't know how to unpack it and even if they do they can not unpack it either due to exceptions. It might be more confusing to them because they don't know what it is and don't know what to do with it.
What I can do though is to add more java docs to this class or the getter to describe what could be null in what scenarios.

I would agree they could not unpack it. And just the fact that there is something encoded in any way gives a developer enough clue that there is a speciifc error and they should be able to unpack it sooner or later. It is better than hiding info completely. Another approach may be instead of storing any, convert it to string and store as a string.

summer-ji-eng

Thanks @blakeli0 for your hard work. Java seems more complicated than other languages.

LGTM on actionable error logic. Please wait for the Java expertise review on language idiom or syntax.

vam-google

Overall looks good, but I'm concerned with the extra dependency in gax module. Gax is a very core dependency of all google cloud libraries, adding something there is a big deal. Looks like up untill this point, gax was not aware of protobuf, and maybe we should keep it this way (i.e. in ErrorDetails do not have those error details classes directly. Maybe have some sort of GrpcErrorDetails in gax-grpc implementing a more generic ErrorDetails from gax.

vam-google · 2022-03-23T18:28:45Z

gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcApiExceptionFactory.java

+
+  private void addErrorDetail(ErrorDetails.Builder errorDetailsBuilder, Any detail) {
+    try {
+      if (detail.is(ErrorInfo.class)) {


Just wondering, is it possible to convert this to a switch statement somehow? For example using class names as strings, as switch accepts strings as the switched value.

I gave it some thoughts as well but couldn't find a good way to use switch statements, I don't think using class names as String would work either. Because we are not checking a variable, we are checking the result of method call, and the input of the method call is a variable. We can only get the type of Any through this method call instead of an attribute, otherwise we could've check it easily.
Let me know if you have some other good ideas.

vam-google · 2022-03-23T18:30:24Z

gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcApiExceptionFactory.java

+    } catch (InvalidProtocolBufferException e) {
+      // If unpacking one of the error detail fails, it should not block unpacking other error
+      // details so that end users can still get some info back. Hence, we don't need to do
+      // anything.


Can we in his case just store the raw Any (not unpacked) detail object in ErrorDetails? Doing empyt error details will just loose information for users and make the error even more confusing to them.

gax/BUILD.bazel

gax/src/main/java/com/google/api/gax/rpc/ApiException.java

gax/src/main/java/com/google/api/gax/rpc/ApiExceptionFactory.java

vam-google · 2022-03-23T18:53:35Z

gax/src/main/java/com/google/api/gax/rpc/CancelledException.java

+
+  public CancelledException(
+      Throwable cause, StatusCode statusCode, boolean retryable, ErrorDetails errorDetails) {
+    super(cause, statusCode, retryable, errorDetails);


yep. It is a typical approach: to have some sort of a "master" consturctor and make other constructors simply call the big one. The master constructor can even be private, if we don't want to expose it.

vam-google

As I mentioned, in my previous comment overall it looks good, but we need to figure out the dependency problem first and placement of the new classes. Putting "request changes" here to prevent accidental push of this PR before we figure out the deps part.

vam-google

LGTM, but please address the remaining comments first.

Looks like I have to accept the new dependencies thing, but please check the comments (and i have a few questions there).

For the new methods in ApiExcepton - please strongly consider removing them. The cross-language desing usually never dictates such implementation-specific things. @summer-ji-eng please advaice.

vam-google · 2022-03-28T17:24:44Z

gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcApiExceptionFactory.java

+    } catch (InvalidProtocolBufferException e) {
+      // If unpacking one of the error detail fails, it should not block unpacking other error
+      // details so that end users can still get some info back. Hence, we don't need to do
+      // anything.


I would agree they could not unpack it. And just the fact that there is something encoded in any way gives a developer enough clue that there is a speciifc error and they should be able to unpack it sooner or later. It is better than hiding info completely. Another approach may be instead of storing any, convert it to string and store as a string.

vam-google · 2022-03-28T17:34:43Z

gax/build.gradle

@@ -5,7 +5,8 @@ project.version = "2.12.3-SNAPSHOT" // {x-version-update:gax:current}

 dependencies {
  api(libraries['maven.com_google_api_api_common'],
-    libraries['maven.com_google_auth_google_auth_library_credentials'],
+          libraries['maven.com_google_api_grpc_proto_google_common_protos'],
+          libraries['maven.com_google_auth_google_auth_library_credentials'],


Why is this one needed?

You mean com_google_auth_google_auth_library_credentials? I think It's a formatting/git issue, I added com_google_api_grpc_proto_google_common_protos but somehow git thinks I removed one and added two.

gax/BUILD.bazel

meltsufin

LGTM. Just one minor comment on adding a message to ProtocolBufferParsingException.

gax/src/main/java/com/google/api/gax/rpc/ErrorDetails.java

sonarqubecloud · 2022-04-01T18:34:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

98.6% Coverage
0.0% Duplication

sonarqubecloud · 2022-04-04T14:56:48Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

98.6% Coverage
0.0% Duplication

Add ErrorDetails to ApiException.

a9f5519

vam-google reviewed Mar 16, 2022

View reviewed changes

meltsufin reviewed Mar 16, 2022

View reviewed changes

blakeli0 added 5 commits March 18, 2022 13:23

Add unit tests.

eba12fb

Add dependency for protobuf-java. Add more unit tests. Add Javadoc.

b53d019

Merge branch 'main' into actionable-error-grpc

5c0fdbe

Fix broken unit tests.

eebc8ee

Add @VisibleForTesting to package private field in GrpcApiExceptionFa…

ee77664

…ctory

blakeli0 requested review from meltsufin, vam-google, summer-ji-eng and eaball35 March 21, 2022 01:35

blakeli0 marked this pull request as ready for review March 21, 2022 01:37

blakeli0 requested review from a team as code owners March 21, 2022 01:37

blakeli0 linked an issue Mar 21, 2022 that may be closed by this pull request

Error Details Improvements - GRPC #1635

Closed

blakeli0 changed the title ~~Add ErrorDetails to ApiException.~~ feat: Error Details Improvements - GRPC Mar 21, 2022

eaball35 reviewed Mar 21, 2022

View reviewed changes

Merge branch 'main' into actionable-error-grpc

1e04734

summer-ji-eng approved these changes Mar 22, 2022

View reviewed changes

Delete unused enum class.

fb71a9d

vam-google reviewed Mar 23, 2022

View reviewed changes

vam-google suggested changes Mar 23, 2022

View reviewed changes

Make ErrorDetails final in ApiException

9f99723

vam-google approved these changes Mar 28, 2022

View reviewed changes

blakeli0 added 3 commits March 30, 2022 16:02

Move the unpacking logic from GrpcApiExceptionFactory to ErrorDetails

7cb4382

Add more tests and java docs.

a1ef130

Merge branch 'main' into actionable-error-grpc

6327bed

meltsufin approved these changes Apr 1, 2022

View reviewed changes

gax/src/main/java/com/google/api/gax/rpc/ErrorDetails.java Outdated Show resolved Hide resolved

blakeli0 added 2 commits April 1, 2022 14:30

Add message to ProtocolBufferParsingException.

7b52a2d

Merge branch 'main' into actionable-error-grpc

f1f54ec

Rename getters in ErrorDetails

184e5d6

blakeli0 merged commit 00c3b9d into main Apr 4, 2022

blakeli0 deleted the actionable-error-grpc branch April 4, 2022 15:20

release-please bot mentioned this pull request Apr 4, 2022

chore(main): release 2.15.0 #1649

Merged

blakeli0 mentioned this pull request Feb 8, 2023

Error Details Improvements - HTTP/JSON googleapis/sdk-platform-java#1321

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Error Details Improvements - GRPC #1634

feat: Error Details Improvements - GRPC #1634

blakeli0 commented Mar 15, 2022 •

edited

Loading

vam-google Mar 16, 2022

blakeli0 Mar 21, 2022

vam-google Mar 23, 2022

blakeli0 Mar 23, 2022

eaball35 Mar 21, 2022

blakeli0 Mar 22, 2022

vam-google Mar 23, 2022

blakeli0 Mar 23, 2022

vam-google Mar 28, 2022

summer-ji-eng left a comment

vam-google left a comment

vam-google Mar 23, 2022

blakeli0 Mar 23, 2022

vam-google Mar 23, 2022

vam-google Mar 23, 2022

vam-google left a comment

vam-google left a comment

vam-google Mar 28, 2022

vam-google Mar 28, 2022

blakeli0 Mar 31, 2022 •

edited

Loading

meltsufin left a comment

sonarqubecloud bot commented Apr 1, 2022

sonarqubecloud bot commented Apr 4, 2022

feat: Error Details Improvements - GRPC #1634

feat: Error Details Improvements - GRPC #1634

Conversation

blakeli0 commented Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

summer-ji-eng left a comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 1, 2022

sonarqubecloud bot commented Apr 4, 2022

blakeli0 commented Mar 15, 2022 •

edited

Loading

blakeli0 Mar 31, 2022 •

edited

Loading